Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse #8151

madhukar93 · 2023-10-26T12:27:31Z

Description

This PR addresses #7837. It adds all fields to Traffic type response that are required while adding traffic type to a physical network using addTrafficType command

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

How Has This Been Tested?

added unit test for response creation
added marvin test
manually tested on simulator with cmk

MySQL root@(none):cloud> update physical_network_traffic_types set vlan="100-200" where physical_network_id=200;

(localcloud) 🐱 > list traffictypes physicalnetworkid=05f91607-18a6-40f5-ac8c-6ee86a052e0e
{
  "count": 4,
  "traffictype": [
    {
      "id": "51821c55-b0d6-49d1-ad3b-da53a90822d4",
      "isolationmethods": "VLAN",
      "physicalnetworkid": "05f91607-18a6-40f5-ac8c-6ee86a052e0e",
      "traffictype": "Guest",
      "vlan": "100-200"
    },
...
  ]
}

How did you try to break this feature and the system with this change?

Nothing besides running test automation

boring-cyborg · 2023-10-26T12:27:36Z

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
Be sure to read the CloudStack Coding Conventions.
Apache CloudStack is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: [email protected] (https://cloudstack.apache.org/mailing-lists.html)
Slack: https://apachecloudstack.slack.com/

server/src/main/java/com/cloud/network/NetworkServiceImpl.java

api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java

rohityadavcloud

left some remarks

api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java

server/src/main/java/com/cloud/api/ApiResponseHelper.java

server/src/main/java/com/cloud/network/NetworkServiceImpl.java

rohityadavcloud · 2023-10-27T07:27:28Z

@madhukar93 you can also consider to add a unit test and/or a functional test to cover the API response.

codecov · 2023-10-27T07:38:27Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (26b01f6) 27.85% compared to head (cc72ada) 22.45%.
Report is 18 commits behind head on main.

Files	Patch %	Lines
...e/cloudstack/api/response/TrafficTypeResponse.java	66.66%	2 Missing ⚠️
...src/main/java/com/cloud/api/ApiResponseHelper.java	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8151      +/-   ##
============================================
- Coverage     27.85%   22.45%   -5.41%     
+ Complexity    29309    22441    -6868     
============================================
  Files          5195     5061     -134     
  Lines        366568   344607   -21961     
  Branches      53597    49627    -3970     
============================================
- Hits         102109    77375   -24734     
- Misses       250398   256079    +5681     
+ Partials      14061    11153    -2908

Flag	Coverage Δ
simulator-marvin-tests	`24.03% <66.66%> (+0.67%)`	⬆️
uitests	`4.49% <ø> (ø)`
unit-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

madhukar93 · 2023-10-27T07:59:23Z

@madhukar93 you can also consider to add a unit test and/or a functional test to cover the API response.

will do after clarifying the schema

api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListTrafficTypesCmd.java

madhukar93

@rohityadavcloud There don't seem to be too many functional tests for the cloud-api package to refer. Need suggestions for what the test scenario should be. Just validating the response schema would be very close the what the unit test is already checking.

harikrishna-patnala · 2023-10-30T04:57:22Z

@blueorangutan package

blueorangutan · 2023-10-30T04:58:03Z

@harikrishna-patnala a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-10-30T05:55:11Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7562

harikrishna-patnala

LGTM @madhukar93 can you check if we can add a small marvin test.

rohityadavcloud · 2023-10-30T07:47:51Z

@madhukar93 the integration test scenario would to be call the API and check the response against expected values of the API response. When changing the response type, you may need to make changes to Marvin library if a utility is used to call the api and process response; otherwise you would use the API client to make the API call and from its response object you'd get keys/entities to compare. When you rebuilt cloudstack (entirely), apidocs and marvin API stubs will be regenerated (which covers the case of say adding new api param or responses, or changing response type class etc).

DaanHoogland

looks mostly good, one remark and needs testing

server/src/main/java/com/cloud/api/ApiResponseHelper.java

shwstppr · 2023-11-17T16:22:35Z

@harikrishna-patnala @kishankavala @rohityadavcloud @vishesh92 are your comments addressed?

@blueorangutan package

blueorangutan · 2023-11-17T16:24:04Z

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-11-17T17:24:52Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7789

shwstppr · 2023-11-17T17:32:37Z

@blueorangutan test

blueorangutan · 2023-11-17T17:34:04Z

@shwstppr a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2023-11-18T07:27:40Z

[SF] Trillian test result (tid-8363)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48520 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8151-t8363-kvm-centos7.zip
Smoke tests completed. 118 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_07_deploy_kubernetes_ha_cluster	`Failure`	3609.78	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	86.89	test_kubernetes_clusters.py

…e.isolationMethods

Co-authored-by: dahn <[email protected]>

DaanHoogland · 2023-11-30T13:30:32Z

@blueorangutan package

blueorangutan · 2023-11-30T13:32:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-11-30T14:29:50Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 7874

DaanHoogland · 2023-11-30T15:25:15Z

@blueorangutan test

blueorangutan · 2023-11-30T15:26:04Z

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2023-12-01T07:43:16Z

[SF] Trillian test result (tid-8436)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 57136 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8151-t8436-kvm-centos7.zip
Smoke tests completed. 116 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_trigger_shutdown	`Failure`	336.99	test_safe_shutdown.py
test_01_invalid_upgrade_kubernetes_cluster	`Failure`	3606.30	test_kubernetes_clusters.py
test_02_upgrade_kubernetes_cluster	`Failure`	3606.46	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_04_autoscale_kubernetes_cluster	`Failure`	0.03	test_kubernetes_clusters.py
test_05_basic_lifecycle_kubernetes_cluster	`Failure`	0.03	test_kubernetes_clusters.py
test_06_delete_kubernetes_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_08_upgrade_kubernetes_ha_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.05	test_kubernetes_clusters.py
test_10_vpc_tier_kubernetes_cluster	`Failure`	51.95	test_kubernetes_clusters.py
test_11_test_unmanaged_cluster_lifecycle	`Error`	0.24	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	56.69	test_kubernetes_clusters.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	307.97	test_hostha_kvm.py

shwstppr · 2023-12-06T11:19:38Z

@vishesh92 @harikrishna-patnala @kishankavala are your concerns addressed. Can you please re-review?

github-actions · 2023-12-07T07:37:39Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

boring-cyborg bot added component:api component:networking labels Oct 26, 2023

madhukar93 marked this pull request as draft October 26, 2023 12:28

madhukar93 commented Oct 26, 2023

View reviewed changes

server/src/main/java/com/cloud/network/NetworkServiceImpl.java Outdated Show resolved Hide resolved

madhukar93 changed the title ~~Fixes:7863: Add isolationMethods and vlan to TrafficTypeResponse~~ Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse Oct 26, 2023

madhukar93 changed the title ~~Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse~~ (WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse Oct 26, 2023

madhukar93 commented Oct 26, 2023

View reviewed changes

api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java Show resolved Hide resolved

madhukar93 commented Oct 26, 2023

View reviewed changes

api/src/main/java/org/apache/cloudstack/api/response/TrafficTypeResponse.java Outdated Show resolved Hide resolved

madhukar93 force-pushed the listTrafficType-7863 branch from 1c10c58 to 0b1950b Compare October 26, 2023 13:30

rohityadavcloud requested changes Oct 27, 2023

View reviewed changes

madhukar93 force-pushed the listTrafficType-7863 branch 2 times, most recently from 18714f8 to 23d1b77 Compare October 27, 2023 12:39

madhukar93 marked this pull request as ready for review October 28, 2023 11:18

madhukar93 changed the title ~~(WIP) Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse~~ Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse Oct 28, 2023

madhukar93 commented Oct 28, 2023

View reviewed changes

api/src/main/java/org/apache/cloudstack/api/command/admin/usage/ListTrafficTypesCmd.java Show resolved Hide resolved

madhukar93 commented Oct 28, 2023

View reviewed changes

harikrishna-patnala reviewed Oct 30, 2023

View reviewed changes

rohityadavcloud requested a review from DaanHoogland October 30, 2023 08:21

DaanHoogland reviewed Oct 30, 2023

View reviewed changes

server/src/main/java/com/cloud/api/ApiResponseHelper.java Outdated Show resolved Hide resolved

boring-cyborg bot added component:integration-test Python Warning... Python code Ahead! labels Nov 4, 2023

madhukar93 force-pushed the listTrafficType-7863 branch from aa14677 to cbba3d3 Compare November 4, 2023 06:26

DaanHoogland closed this Nov 18, 2023

DaanHoogland reopened this Nov 18, 2023

madhukar93 force-pushed the listTrafficType-7863 branch from 305af22 to bf7bd8c Compare November 30, 2023 11:16

madhukar93 requested a review from rohityadavcloud November 30, 2023 11:23

madhukar93 and others added 10 commits November 30, 2023 18:27

add vlan, isolationMethod to TrafficTypeResponse

32cfb2d

set vlan, isolationMethods in createTrafficTypeResponse()

d4460e7

change PhysicalNetworkTrafficType.isolationMethods -> string

8c412f6

Add unit test for createTrafficTypeResponse

d8cc5cc

Fix responseObject on ListTrafficTypesCmd

0777b38

don't depend on PhysicalNetworkResponse for PhysicalNetworkTrafficTyp…

7c308b2

…e.isolationMethods

Add marvin test for TrafficTypeResponse

4c8dfb6

add license

bb476dc

Co-authored-by: dahn <[email protected]>

add teardown for TestTrafficTypeApi

1df0bb1

Co-authored-by: dahn <[email protected]>

fix style

cc72ada

Co-authored-by: dahn <[email protected]>

madhukar93 force-pushed the listTrafficType-7863 branch from bf7bd8c to cc72ada Compare November 30, 2023 13:00

github-actions bot added the status:has-conflicts label Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse #8151

Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse #8151

madhukar93 commented Oct 26, 2023 •

edited

Loading

boring-cyborg bot commented Oct 26, 2023

rohityadavcloud left a comment

rohityadavcloud commented Oct 27, 2023

codecov bot commented Oct 27, 2023 •

edited

Loading

madhukar93 commented Oct 27, 2023 •

edited

Loading

madhukar93 left a comment

harikrishna-patnala commented Oct 30, 2023

blueorangutan commented Oct 30, 2023

blueorangutan commented Oct 30, 2023

harikrishna-patnala left a comment

rohityadavcloud commented Oct 30, 2023

DaanHoogland left a comment

shwstppr commented Nov 17, 2023

blueorangutan commented Nov 17, 2023

blueorangutan commented Nov 17, 2023

shwstppr commented Nov 17, 2023

blueorangutan commented Nov 17, 2023

blueorangutan commented Nov 18, 2023

DaanHoogland commented Nov 30, 2023

blueorangutan commented Nov 30, 2023

blueorangutan commented Nov 30, 2023

DaanHoogland commented Nov 30, 2023

blueorangutan commented Nov 30, 2023

blueorangutan commented Dec 1, 2023

shwstppr commented Dec 6, 2023

github-actions bot commented Dec 7, 2023

Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse #8151

Are you sure you want to change the base?

Fixes:#7837: Add isolationMethods and vlan to TrafficTypeResponse #8151

Conversation

madhukar93 commented Oct 26, 2023 • edited Loading

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

How Has This Been Tested?

How did you try to break this feature and the system with this change?

boring-cyborg bot commented Oct 26, 2023

rohityadavcloud left a comment

Choose a reason for hiding this comment

rohityadavcloud commented Oct 27, 2023

codecov bot commented Oct 27, 2023 • edited Loading

Codecov Report

madhukar93 commented Oct 27, 2023 • edited Loading

madhukar93 left a comment

Choose a reason for hiding this comment

harikrishna-patnala commented Oct 30, 2023

blueorangutan commented Oct 30, 2023

blueorangutan commented Oct 30, 2023

harikrishna-patnala left a comment

Choose a reason for hiding this comment

rohityadavcloud commented Oct 30, 2023

DaanHoogland left a comment

Choose a reason for hiding this comment

shwstppr commented Nov 17, 2023

blueorangutan commented Nov 17, 2023

blueorangutan commented Nov 17, 2023

shwstppr commented Nov 17, 2023

blueorangutan commented Nov 17, 2023

blueorangutan commented Nov 18, 2023

DaanHoogland commented Nov 30, 2023

blueorangutan commented Nov 30, 2023

blueorangutan commented Nov 30, 2023

DaanHoogland commented Nov 30, 2023

blueorangutan commented Nov 30, 2023

blueorangutan commented Dec 1, 2023

shwstppr commented Dec 6, 2023

github-actions bot commented Dec 7, 2023

madhukar93 commented Oct 26, 2023 •

edited

Loading

codecov bot commented Oct 27, 2023 •

edited

Loading

madhukar93 commented Oct 27, 2023 •

edited

Loading